-
-
Notifications
You must be signed in to change notification settings - Fork 367
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix inventory renaming error. #6159
Conversation
Co-authored-by: Patrick Miller <apickledwalrus@gmail.com>
Co-authored-by: Patrick Miller <apickledwalrus@gmail.com>
Co-authored-by: Patrick Miller <apickledwalrus@gmail.com>
Co-authored-by: Patrick Miller <apickledwalrus@gmail.com>
Co-authored-by: Patrick Miller <apickledwalrus@gmail.com>
Co-authored-by: Patrick Miller <apickledwalrus@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concerns aren't blocking, though they should be addressed somehow.
return null; | ||
return Bukkit.createInventory(null, type); | ||
} | ||
return object; // Return the same ItemType they passed without applying a name. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other get
below handles converting ItemStacks to ItemTypes. Do we need to handle that here too (to avoid an ArrayStoreException)? I'm not actually sure that ItemStacks could even be passed into this method though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know, TheLimeGlass didn't do this in the original PR but there was another change made to this class since that added that converter outside the getter, so I merged the changes together but since I don't know what/why that's there I can't say whether it's needed.
if (Skript.isRunningMinecraft(1, 14) && type == InventoryType.COMPOSTER) | ||
return false; | ||
if (Skript.isRunningMinecraft(1, 20) && type == InventoryType.CHISELED_BOOKSHELF) | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
caching still maybe a good idea ;)
@SuppressWarnings("unchecked") | ||
public boolean init(Expression<?>[] exprs, int matchedPattern, Kleenean isDelayed, ParseResult parseResult) { | ||
if (exprs[0].getReturnType().equals(ItemStack.class)) { | ||
setExpr(exprs[0].getConvertedExpression(ItemType.class)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this needed? Can you add a comment explaining why it is if so
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no idea what it is, what it does or why it's there, I didn't write it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Existing code had casting to ensure the type was not an ItemStack. For some reason in the past an ItemType could have been an ItemStack. This enforces that and removes the need for casting in runtime. This was discussed on the skriptlang Discord a long time ago to do it this way.
Refactored #5943 for dev/patch if that's the reasoning for this pull |
@Moderocky @TheLimeGlass |
Closing in favour of #5943 |
Description
Prevents attempts at creating a non-creatable inventory type.
Also (actually) fixes an error when trying to rename inventory types.
A cherry pick of #5943 for patch branch with extra bugs fixed.
Target Minecraft Versions: any
Requirements: none
Related Issues: #5495 and #5923